Remove mouse enter/leave polyfill and add native event #10247
Conversation
| @@ -64,6 +64,8 @@ var topLevelTypes = { | |||
| topLoadedMetadata: 'loadedmetadata', | |||
| topLoadStart: 'loadstart', | |||
| topMouseDown: 'mousedown', | |||
| topMouseEnter: 'mouseenter', | |||
| topMouseLeave: 'mouseleave', | |||
There was a problem hiding this comment.
why does this file still exist? It seems to be mostly (only?) used as a type import, but has actual code in it. Does this make it into a build? maybe worth changing to a purely flow file so there isn't any worry of runtime usage?
There was a problem hiding this comment.
topLevelTypes is imported in ReactBrowserEventEmitter, see: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactBrowserEventEmitter.js#L19
There was a problem hiding this comment.
I wonder how irregular the mapping is, could maybe remove it in favor of something like we do here: https://github.com/jquense/react/blob/2a69ba16caf4eee8c9f1dcfbeb937049c494cec3/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js#L133-L135
| dependency === 'topMouseEnter' || | ||
| dependency === 'topMouseLeave' | ||
| ) { | ||
| if (isEventSupported('mouseenter', true)) { |
There was a problem hiding this comment.
If we're assuming all supported browsers will support mouseenter events, do we need the isEventSupported check?
| }; | ||
| } else { | ||
| type.registrationName = onEvent; | ||
| } |
There was a problem hiding this comment.
This makes the type object polymorphic which could hurt performance a bit. We may want to define these fields on type with null values to avoid creating multiple hidden classes.
|
This is great! I love seeing all that red 😄
I think two plugins might be cleaner, but I'm not sure it's justified if |
Yeah i'm not sure how many other events might fit there, perhaps native |
|
I think inlining the logic in SimpleEventPlugin is fine for now, and we can revisit when we have a stronger case for DirectEventPlugin. |
|
I can't get jests watch mode to work with the new project setup... making it tough to debug the CI failure |
jquense
force-pushed
the
mouse-enter-leave-2
branch
from
2a69ba1 to
b92e54c
Compare
|
OK I added a portal fixture, and its broken. I'm guessing what we have to do is prevent a dispatch if the relatedTarget is a portal of the current target. That is easy enough to do in an EventPlugin, however the case where you leave the portal, ultimately the portal at the same time is a bit harder, be cause you need to find any ancestors listening for ML, calculate if they are being left as well and trigger them at that point. @sebmarkbage do you have thoughts on where start with something like this? I'd be concerned that we will end up writing a full polyfill anway way to get that behavior. Perhaps though it could be pushed to the propagators? I think that would require they have knowledge of the paired events in the ME case. |
|
We'll probably need to decide pretty quickly if we want to revert the behavior in the 16 cycle. One way to make that decision could be to make an attempt at a polyfill using the dispatch mechanism to show that it would be prohibitively big. |
|
Maybe it's ok to treat this as if the user just moved out and quickly back in again? |
|
Is this the only pair of events like this? e.g. non-bubbling events that "own" an entire subtree? The only other use-case for this behavior I can think of is something like "focusenter" and "focusleave" but those aren't real events (though there has been some talk of implementing something like it, b/c its useful). I ask because the Portal bubbling is really helpful for lots of other cases. If this is the only hard case, i'd say it's better to have this inconsistency vs reverting the behavior entirely. I'll take shot at implementing it here to see what would be required tho |
|
de un jlit pas langlais jmen fou un peu dsl si sa vous a choquer le ponpon
!! crêpe toi pas le chignion pour sa !!! Nice day !!
Le 2017-07-24 15:18, "Jason Quense" <[email protected]> a écrit :
… Is this the only pair of events like this? e.g. non-bubbling events that
"own" an entire subtree? The only other use-case for this behavior I can
think of is something like "focusenter" and "focusleave" but those aren't
real events (though there has been some talk of implementing something like
it, b/c its useful). I ask because the Portal bubbling is really helpful
for lots of other cases. If this is the only hard case, i'd say it's better
to have this inconsistency vs reverting the behavior entirely.
I'll take shot at implementing it here to see what would be required tho
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10247 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AdAYsrBeCF1cPMGg6-pm3X1klkt9PlDyks5sRO3_gaJpZM4Ofhw9>
.
|
|
Closing the PR as stale, we can track this in #11972. |
🔥 rel: #9824
I’m starting with this, and getting the tests to pass. I’m a bit unsure if there is more needed for the Portal case, but I believe the existing tests for that pass right now.
I wasn’t sure if complicating SimpleEventPlugin made sense, or if maybe it’s worth splitting that into two: PhasedEventPlugin and DirectEventPlugin or something.